feat(common): dual-server db validation for byod config - #371
Open
yash-pouranik wants to merge 1 commit into
Open
feat(common): dual-server db validation for byod config#371yash-pouranik wants to merge 1 commit into
yash-pouranik wants to merge 1 commit into
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe PR adds an authenticated public API endpoint for temporary MongoDB connectivity checks, updates dashboard-side verification and error reporting, and retrieves server IPs from both dashboard and public API endpoints. ChangesExternal database verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DashboardForm
participant DashboardAPI
participant PublicAPI
participant MongoDB
DashboardForm->>DashboardAPI: Submit external database configuration
DashboardAPI->>MongoDB: Verify local dbUri
DashboardAPI->>PublicAPI: Optionally verify dbUri remotely
PublicAPI->>MongoDB: Verify temporary connection
PublicAPI-->>DashboardAPI: Return verification result and serverIp
DashboardAPI-->>DashboardForm: Return configuration result and access guidance
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
apps/public-api/src/routes/internal.js (2)
9-20: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTiming-unsafe secret comparison.
providedSecret !== secretis a variable-time string comparison, which is a known side-channel weakness for secret verification.🔒 Use constant-time comparison
+const crypto = require('crypto'); + const internalAuth = (req, res, next) => { const secret = process.env.INTERNAL_SECRET; if (!secret) { console.error("INTERNAL_SECRET is not configured on Public API"); return res.status(500).json({ success: false, message: "Server misconfiguration" }); } const providedSecret = req.headers['x-internal-secret']; - if (!providedSecret || providedSecret !== secret) { + const provided = Buffer.from(String(providedSecret || '')); + const expected = Buffer.from(secret); + if (provided.length !== expected.length || !crypto.timingSafeEqual(provided, expected)) { return res.status(403).json({ success: false, message: "Forbidden: Invalid internal secret" }); } next(); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/public-api/src/routes/internal.js` around lines 9 - 20, Replace the variable-time providedSecret !== secret check in the internal-secret middleware with a constant-time comparison using the appropriate crypto API, ensuring both values are safely normalized to equal-length byte representations before comparison. Preserve the existing 403 response for missing or invalid secrets and the next() path for valid credentials.
30-51: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTemp MongoDB connections aren't closed on the failure path in either service. Both files create a
mongoose.createConnection(...)for verification but only call.close()on success; the same cleanup gap exists in both.
apps/public-api/src/routes/internal.js#L30-L51: closetempConnin afinallyblock covering both the success and catch paths.apps/dashboard-api/src/controllers/project.controller.js#L734-L738: apply the samefinally-based cleanup to its localtempConn.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/public-api/src/routes/internal.js` around lines 30 - 51, Ensure both verification flows close their temporary MongoDB connections on success and failure. In apps/public-api/src/routes/internal.js lines 30-51, move tempConn cleanup into a finally block surrounding the try/catch and remove the success-only close; apply the same finally-based cleanup to the local tempConn in apps/dashboard-api/src/controllers/project.controller.js lines 734-738.apps/public-api/src/app.js (1)
82-84: 🔒 Security & Privacy | 🔵 TrivialConsider network-level restriction as defense in depth.
/api/internalrelies solely on a shared secret header for authorization. If feasible, also restrict this path at the infrastructure layer (VPC/security group, ingress rule) so it's unreachable from the public internet, complementing the secret check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/public-api/src/app.js` around lines 82 - 84, Restrict the /api/internal route exposed by app.use and the internalRoute registration to trusted internal network sources at the infrastructure layer, such as VPC, security-group, or ingress rules, while retaining the existing shared-secret authorization. Ensure the path is unreachable from the public internet without changing other routes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/dashboard-api/src/controllers/project.controller.js`:
- Around line 726-750: Move the getPublicIp() call out of the main try block and
into the catch block, where dashboardIp is only needed for failure reporting.
Keep the database connection and Public API verification flow unchanged, and
preserve lazy handling consistent with publicApiIp.
- Around line 730-777: Bound the verification flow in the surrounding controller
handler with an overall timeout so local MongoDB verification, remote
`/api/internal/test-db` verification, and fallback `/api/server-ip` lookup
cannot cumulatively exceed the request deadline. Preserve the existing error
classification and IP propagation while ensuring the timeout rejects promptly
and is handled by the existing connection-failure path.
- Around line 741-749: Update updateExternalConfig around the PUBLIC_API_URL and
INTERNAL_SECRET check so external DB confirmation cannot return success when
verification is skipped. Require both configuration values for external DB
updates and fail the operation when either is missing, unless an existing
explicit opt-out is provided; preserve the current axios verification path when
both values are present.
- Around line 755-760: Update the Axios error handling in the project connection
flow around connErr.response.data so only database-URI-specific verification
failures use the returned message; replace internal authentication,
configuration, and other infrastructure errors with a safe user-facing fallback
indicating public API verification failed. Preserve the existing serverIp
extraction behavior.
In `@apps/public-api/src/routes/internal.js`:
- Line 11: Normalize every response in the internal route handlers, including
the branches near the referenced lines, to the mandated { success, data, message
} shape by always including data as an object. Move the failure response’s
serverIp value into data.serverIp, and preserve the existing success status,
message, and payload semantics within data.
- Around line 22-34: Validate dbUri with the existing isSafeUri
restricted-host/internal-network check before mongoose.createConnection in the
/test-db route. Share or reuse that validator from a common module so this
endpoint independently rejects unsafe URIs with an appropriate client error,
while preserving the current connection test for safe URIs.
In `@apps/web-dashboard/src/components/Settings/DatabaseConfigForm.jsx`:
- Around line 24-33: Update the direct public API request in the server-IP
loading flow around Promise.allSettled to use an AbortController timeout and
validate response.ok before parsing JSON. Ensure a hung request is rejected
within the timeout so the fulfilled dashboard API result can still update
serverIp, while preserving the existing mounted-state and IP aggregation
behavior.
---
Nitpick comments:
In `@apps/public-api/src/app.js`:
- Around line 82-84: Restrict the /api/internal route exposed by app.use and the
internalRoute registration to trusted internal network sources at the
infrastructure layer, such as VPC, security-group, or ingress rules, while
retaining the existing shared-secret authorization. Ensure the path is
unreachable from the public internet without changing other routes.
In `@apps/public-api/src/routes/internal.js`:
- Around line 9-20: Replace the variable-time providedSecret !== secret check in
the internal-secret middleware with a constant-time comparison using the
appropriate crypto API, ensuring both values are safely normalized to
equal-length byte representations before comparison. Preserve the existing 403
response for missing or invalid secrets and the next() path for valid
credentials.
- Around line 30-51: Ensure both verification flows close their temporary
MongoDB connections on success and failure. In
apps/public-api/src/routes/internal.js lines 30-51, move tempConn cleanup into a
finally block surrounding the try/catch and remove the success-only close; apply
the same finally-based cleanup to the local tempConn in
apps/dashboard-api/src/controllers/project.controller.js lines 734-738.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a0085f2-567e-4508-a558-433bf643fa84
📒 Files selected for processing (4)
apps/dashboard-api/src/controllers/project.controller.jsapps/public-api/src/app.jsapps/public-api/src/routes/internal.jsapps/web-dashboard/src/components/Settings/DatabaseConfigForm.jsx
Comment on lines
+726
to
+750
|
|
||
| let dashboardIp = null; | ||
| let publicApiIp = null; | ||
|
|
||
| try { | ||
| dashboardIp = await getPublicIp(); | ||
|
|
||
| // 1. Test local connection (Dashboard API) | ||
| const tempConn = mongoose.createConnection(dbUri, { | ||
| serverSelectionTimeoutMS: 5000, | ||
| }); | ||
| await tempConn.asPromise(); | ||
| await tempConn.close(); | ||
|
|
||
| // 2. Test remote connection (Public API) if PUBLIC_API_URL is configured | ||
| if (process.env.PUBLIC_API_URL && process.env.INTERNAL_SECRET) { | ||
| const publicApiRes = await axios.post(`${process.env.PUBLIC_API_URL}/api/internal/test-db`, | ||
| { dbUri }, | ||
| { headers: { 'x-internal-secret': process.env.INTERNAL_SECRET }, timeout: 8000 } | ||
| ); | ||
| // If it didn't throw, it succeeded. | ||
| } else { | ||
| console.warn("Skipping Public API DB verification because PUBLIC_API_URL or INTERNAL_SECRET is missing."); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Eager getPublicIp() call risks misattributing unrelated failures as DB connection errors.
dashboardIp is fetched unconditionally before the connection test even starts, inside the same try block. If getPublicIp() itself fails/throws, the catch block will report it as "Could not connect to the provided MongoDB URI," which is misleading. It's also wasted work on the happy path since dashboardIp is only used in the catch block. Consider fetching it lazily inside catch, mirroring how publicApiIp is already fetched lazily (L769-777).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/dashboard-api/src/controllers/project.controller.js` around lines 726 -
750, Move the getPublicIp() call out of the main try block and into the catch
block, where dashboardIp is only needed for failure reporting. Keep the database
connection and Public API verification flow unchanged, and preserve lazy
handling consistent with publicApiIp.
Comment on lines
730
to
+777
| try { | ||
| dashboardIp = await getPublicIp(); | ||
|
|
||
| // 1. Test local connection (Dashboard API) | ||
| const tempConn = mongoose.createConnection(dbUri, { | ||
| serverSelectionTimeoutMS: 5000, | ||
| }); | ||
| await tempConn.asPromise(); | ||
| await tempConn.close(); | ||
|
|
||
| // 2. Test remote connection (Public API) if PUBLIC_API_URL is configured | ||
| if (process.env.PUBLIC_API_URL && process.env.INTERNAL_SECRET) { | ||
| const publicApiRes = await axios.post(`${process.env.PUBLIC_API_URL}/api/internal/test-db`, | ||
| { dbUri }, | ||
| { headers: { 'x-internal-secret': process.env.INTERNAL_SECRET }, timeout: 8000 } | ||
| ); | ||
| // If it didn't throw, it succeeded. | ||
| } else { | ||
| console.warn("Skipping Public API DB verification because PUBLIC_API_URL or INTERNAL_SECRET is missing."); | ||
| } | ||
|
|
||
| } catch (connErr) { | ||
| console.error("Verification Connection Failed:", connErr.message); | ||
| let errorMsg = "Could not connect to the provided MongoDB URI."; | ||
|
|
||
| if ( | ||
|
|
||
| // If the error came from Axios (Public API verification failed) | ||
| if (connErr.response && connErr.response.data && !connErr.response.data.success) { | ||
| errorMsg = connErr.response.data.message; | ||
| if (connErr.response.data.serverIp) { | ||
| publicApiIp = connErr.response.data.serverIp; | ||
| } | ||
| } else if ( | ||
| connErr.message.includes("Server selection timed out") || | ||
| connErr.message.includes("Could not connect") | ||
| ) { | ||
| const serverIp = await getPublicIp(); | ||
| errorMsg = `Access Denied: Please whitelist Server IP [${serverIp}] in MongoDB Atlas.`; | ||
| // Local failure | ||
| errorMsg = `Access Denied: Please whitelist Server IPs in MongoDB Atlas.`; | ||
| } | ||
|
|
||
| // Fetch Public API IP if we don't have it yet, to show both in the error message | ||
| if (!publicApiIp && process.env.PUBLIC_API_URL) { | ||
| try { | ||
| const ipRes = await axios.get(`${process.env.PUBLIC_API_URL}/api/server-ip`, { timeout: 3000 }); | ||
| if (ipRes.data && ipRes.data.ip) publicApiIp = ipRes.data.ip; | ||
| } catch (e) { | ||
| console.error("Failed to fetch public API IP for error message", e.message); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Sequential verification steps can stack up to ~16s of latency on one request.
Local test (≤5s serverSelectionTimeoutMS) → remote test (≤8s axios timeout) → extra public-IP lookup on failure (≤3s) all run sequentially within a single HTTP request handler, with no overall deadline. This risks gateway/proxy timeouts and a poor UX for a config-save action.
Consider running local/remote checks concurrently where possible, or wrapping the whole verification in an overall timeout (e.g., Promise.race with a bounded deadline) independent of the individual per-call timeouts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/dashboard-api/src/controllers/project.controller.js` around lines 730 -
777, Bound the verification flow in the surrounding controller handler with an
overall timeout so local MongoDB verification, remote `/api/internal/test-db`
verification, and fallback `/api/server-ip` lookup cannot cumulatively exceed
the request deadline. Preserve the existing error classification and IP
propagation while ensuring the timeout rejects promptly and is handled by the
existing connection-failure path.
Comment on lines
+741
to
+749
| if (process.env.PUBLIC_API_URL && process.env.INTERNAL_SECRET) { | ||
| const publicApiRes = await axios.post(`${process.env.PUBLIC_API_URL}/api/internal/test-db`, | ||
| { dbUri }, | ||
| { headers: { 'x-internal-secret': process.env.INTERNAL_SECRET }, timeout: 8000 } | ||
| ); | ||
| // If it didn't throw, it succeeded. | ||
| } else { | ||
| console.warn("Skipping Public API DB verification because PUBLIC_API_URL or INTERNAL_SECRET is missing."); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | grep -F 'apps/dashboard-api/src/controllers/project.controller.js' || true
echo "== file size == "
wc -l apps/dashboard-api/src/controllers/project.controller.js
echo "== outline relevant sections =="
ast-grep outline apps/dashboard-api/src/controllers/project.controller.js --match saveProject --view expanded || true
ast-grep outline apps/dashboard-api/src/controllers/project.controller.js --match validateProjectConnection --view expanded || true
echo "== lines 700-770 =="
sed -n '700,770p' apps/dashboard-api/src/controllers/project.controller.js | cat -n
echo "== relevant usages/test DB endpoint =="
rg -n "PUBLIC_API_URL|INTERNAL_SECRET|/api/internal/test-db|saveProject|validateProjectConnection|test DB|verification|warnings" apps/dashboard-api/src -SRepository: geturbackend/urBackend
Length of output: 8065
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== project.controller.js snippet around rest of updateExternalConfig =="
sed -n '770,835p' apps/dashboard-api/src/controllers/project.controller.js | cat -n
echo "== project.controller.js snippet around tests endpoint =="
rg -n "/api/internal/test-db|test-db" apps -S
echo "== project controller routes for updateExternalConfig =="
rg -n "updateExternalConfig|routes\.projects|/projects/" apps/dashboard-api/src -SRepository: geturbackend/urBackend
Length of output: 5480
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test-db endpoint =="
sed -n '1,80p' apps/public-api/src/routes/internal.js | cat -n
echo "== response shape checks =="
node - <<'JS'
const fs = require('fs');
const path = 'apps/public-api/src/routes/internal.js';
const src = fs.readFileSync(path, 'utf8');
const endpoint = src.split("router.post('/test-db'")[1].split("router.")[0];
console.log('endpoint contains success: true:', endpoint.includes("success: true"));
console.log('endpoint contains success: false:', endpoint.includes("success: false"));
console.log('endpoint contains message:', endpoint.includes("message"));
JS
echo "== dashboard success/warnings paths =="
python3 - <<'PY'
from pathlib import Path
import re
p = Path('apps/dashboard-api/src/controllers/project.controller.js')
text = p.read_text()
for m in re.finditer(r"res\.status\(\d+\)\.json\(\{ success: (true|false), data: }, message: \"[^\"]+\"", text):
print(text[m.start()-50:m.end()+100])
PYRepository: geturbackend/urBackend
Length of output: 2669
Surface or enforce missing Public API DB verification.
When PUBLIC_API_URL or INTERNAL_SECRET is absent, updateExternalConfig logs a warning and then sends success: true for an externalDB confirmation that never ran. This defeats the dual-server validation guarantee; either require both values for external DB updates, or make the call fail closed unless the caller explicitly opts out.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/dashboard-api/src/controllers/project.controller.js` around lines 741 -
749, Update updateExternalConfig around the PUBLIC_API_URL and INTERNAL_SECRET
check so external DB confirmation cannot return success when verification is
skipped. Require both configuration values for external DB updates and fail the
operation when either is missing, unless an existing explicit opt-out is
provided; preserve the current axios verification path when both values are
present.
Comment on lines
+755
to
+760
| // If the error came from Axios (Public API verification failed) | ||
| if (connErr.response && connErr.response.data && !connErr.response.data.success) { | ||
| errorMsg = connErr.response.data.message; | ||
| if (connErr.response.data.serverIp) { | ||
| publicApiIp = connErr.response.data.serverIp; | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Internal API error messages are forwarded to end users verbatim.
errorMsg = connErr.response.data.message trusts whatever internal.js returns. If that call fails for infrastructure reasons (e.g., INTERNAL_SECRET mismatch → "Forbidden: Invalid internal secret", or missing config → "Server misconfiguration"), those internal-only strings get shown directly to the customer configuring their DB, leaking internal auth/config details and confusing users about the actual (their-DB-unrelated) failure cause.
🛡️ Only trust dbUri-specific failure messages
- if (connErr.response && connErr.response.data && !connErr.response.data.success) {
- errorMsg = connErr.response.data.message;
+ if (connErr.response && connErr.response.status === 400 && connErr.response.data && !connErr.response.data.success) {
+ errorMsg = connErr.response.data.message;
if (connErr.response.data.serverIp) {
publicApiIp = connErr.response.data.serverIp;
}
+ } else if (connErr.response) {
+ // 403/500 from the internal route indicate our own infra issue, not the user's DB URI
+ console.error("Internal verification service error:", connErr.response.status, connErr.response.data);
} else if (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/dashboard-api/src/controllers/project.controller.js` around lines 755 -
760, Update the Axios error handling in the project connection flow around
connErr.response.data so only database-URI-specific verification failures use
the returned message; replace internal authentication, configuration, and other
infrastructure errors with a safe user-facing fallback indicating public API
verification failed. Preserve the existing serverIp extraction behavior.
| const secret = process.env.INTERNAL_SECRET; | ||
| if (!secret) { | ||
| console.error("INTERNAL_SECRET is not configured on Public API"); | ||
| return res.status(500).json({ success: false, message: "Server misconfiguration" }); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Responses don't conform to the mandated { success, data, message } shape.
Every response in this file omits data: {}, and the failure response also adds a non-standard top-level serverIp field instead of nesting it under data.
As per coding guidelines, "All API endpoints must return { success: bool, data: {}, message: "" }."
🔧 Normalize response shape
- return res.status(500).json({ success: false, message: "Server misconfiguration" });
+ return res.status(500).json({ success: false, data: {}, message: "Server misconfiguration" });
...
- return res.status(403).json({ success: false, message: "Forbidden: Invalid internal secret" });
+ return res.status(403).json({ success: false, data: {}, message: "Forbidden: Invalid internal secret" });
...
- return res.status(400).json({ success: false, message: "dbUri is required" });
+ return res.status(400).json({ success: false, data: {}, message: "dbUri is required" });
...
- return res.status(200).json({ success: true, message: "Connection verified" });
+ return res.status(200).json({ success: true, data: {}, message: "Connection verified" });
...
- return res.status(400).json({ success: false, message: errorMsg, serverIp });
+ return res.status(400).json({ success: false, data: { serverIp }, message: errorMsg });Also applies to: 16-16, 25-25, 35-35, 49-49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/public-api/src/routes/internal.js` at line 11, Normalize every response
in the internal route handlers, including the branches near the referenced
lines, to the mandated { success, data, message } shape by always including data
as an object. Move the failure response’s serverIp value into data.serverIp, and
preserve the existing success status, message, and payload semantics within
data.
Source: Coding guidelines
Comment on lines
+22
to
+34
| router.post('/test-db', internalAuth, async (req, res) => { | ||
| const { dbUri } = req.body; | ||
| if (!dbUri) { | ||
| return res.status(400).json({ success: false, message: "dbUri is required" }); | ||
| } | ||
|
|
||
| console.log("[Internal] Verifying connection for external DB..."); | ||
| try { | ||
| const tempConn = mongoose.createConnection(dbUri, { | ||
| serverSelectionTimeoutMS: 5000, | ||
| }); | ||
| await tempConn.asPromise(); | ||
| await tempConn.close(); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
No independent SSRF/host validation before opening the connection.
dbUri is trusted as-is here and passed straight to mongoose.createConnection. The only restricted-host/loopback/internal-network check (isSafeUri) lives in apps/dashboard-api/src/controllers/project.controller.js and is never applied on this side. This endpoint effectively becomes an open connectivity prober for any host if reached with an unvalidated URI (secret leak, misconfigured caller, future direct integration), since it will attempt outbound connections to whatever dbUri is supplied.
Recommend duplicating/sharing the restricted-host validation (e.g., move isSafeUri into @urbackend/common so both services enforce it) before calling mongoose.createConnection here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/public-api/src/routes/internal.js` around lines 22 - 34, Validate dbUri
with the existing isSafeUri restricted-host/internal-network check before
mongoose.createConnection in the /test-db route. Share or reuse that validator
from a common module so this endpoint independently rejects unsafe URIs with an
appropriate client error, while preserving the current connection test for safe
URIs.
Comment on lines
+24
to
+33
| const [res1, res2] = await Promise.allSettled([ | ||
| api.get(`/api/server-ip`), | ||
| fetch(`${PUBLIC_API_URL}/api/server-ip`).then(r => r.json()) | ||
| ]); | ||
|
|
||
| const ips = []; | ||
| if (res1.status === 'fulfilled' && res1.value.data.ip) ips.push(res1.value.data.ip); | ||
| if (res2.status === 'fulfilled' && res2.value.ip) ips.push(res2.value.ip); | ||
|
|
||
| if (isMounted) setServerIp(ips.join(', ')); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the public API request.
The new fetch has no timeout. Because Promise.allSettled waits for every promise, a hung public API prevents serverIp from being updated—even when the dashboard API already returned its IP—leaving the whitelist guidance stuck on ....
Add an abort timeout (and check response.ok) for the direct request, or update the state as each successful request completes.
Proposed timeout handling
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), 5000);
const [res1, res2] = await Promise.allSettled([
api.get(`/api/server-ip`),
- fetch(`${PUBLIC_API_URL}/api/server-ip`).then(r => r.json())
+ fetch(`${PUBLIC_API_URL}/api/server-ip`, { signal: controller.signal })
+ .then(r => {
+ if (!r.ok) throw new Error(`HTTP ${r.status}`);
+ return r.json();
+ })
+ .finally(() => clearTimeout(timeoutId))
]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const [res1, res2] = await Promise.allSettled([ | |
| api.get(`/api/server-ip`), | |
| fetch(`${PUBLIC_API_URL}/api/server-ip`).then(r => r.json()) | |
| ]); | |
| const ips = []; | |
| if (res1.status === 'fulfilled' && res1.value.data.ip) ips.push(res1.value.data.ip); | |
| if (res2.status === 'fulfilled' && res2.value.ip) ips.push(res2.value.ip); | |
| if (isMounted) setServerIp(ips.join(', ')); | |
| const controller = new AbortController(); | |
| const timeoutId = setTimeout(() => controller.abort(), 5000); | |
| const [res1, res2] = await Promise.allSettled([ | |
| api.get(`/api/server-ip`), | |
| fetch(`${PUBLIC_API_URL}/api/server-ip`, { signal: controller.signal }) | |
| .then(r => { | |
| if (!r.ok) throw new Error(`HTTP ${r.status}`); | |
| return r.json(); | |
| }) | |
| .finally(() => clearTimeout(timeoutId)) | |
| ]); | |
| const ips = []; | |
| if (res1.status === 'fulfilled' && res1.value.data.ip) ips.push(res1.value.data.ip); | |
| if (res2.status === 'fulfilled' && res2.value.ip) ips.push(res2.value.ip); | |
| if (isMounted) setServerIp(ips.join(', ')); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web-dashboard/src/components/Settings/DatabaseConfigForm.jsx` around
lines 24 - 33, Update the direct public API request in the server-IP loading
flow around Promise.allSettled to use an AbortController timeout and validate
response.ok before parsing JSON. Ensure a hung request is rejected within the
timeout so the fulfilled dashboard API result can still update serverIp, while
preserving the existing mounted-state and IP aggregation behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While adding mongo db connection string in settings, user have to whitelist both the IPs of the microservices.
Summary by CodeRabbit
New Features
Bug Fixes